-
-
Notifications
You must be signed in to change notification settings - Fork 24
fix: Big int handling #1758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Big int handling #1758
Conversation
pkg.pr.new packages
benchmark commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes big integer handling for floating-point literals in the TGSL type system. The fix ensures that very large numeric literals (beyond 2^63) are treated as abstract floats rather than integers, eliminating the need for explicit f32 casts.
- Added a check in
numericLiteralToSnippet
to handle values exceeding 2^63 as abstract floats - Removed unnecessary f32 casts from floating-point constants like
FLT_MAX
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/typegpu/src/tgsl/generationHelpers.ts | Added logic to treat large numeric literals as abstract floats |
packages/typegpu-color/src/oklab.ts | Removed unnecessary f32 cast from FLT_MAX constant |
packages/typegpu/tests/examples/individual/oklab.test.ts | Updated test expectation to match new FLT_MAX format |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
} | ||
|
||
export function numericLiteralToSnippet(value: number): Snippet { | ||
if (Math.abs(value) > 2 ** 63) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about value >= 2 ** 63 || value < -(2 ** 63)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yea, forgot that we take Math.abs
According to your question, I believe if we take |
Numbers exceeding 2**63 are now automatically inferred as abstractFloat, since it could not fit in an abstractInt anyway.
Now the following does not warn, and the cast is unnecessary.
const FLT_MAX = f32(3.40282346e38);
You could argue that we should cast all numbers exceeding safe integers (~2**53) to floats, and that might be correct. Can you help me decide?